Support TiDB NullEQ pushdown#10946
Conversation
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
DeltaMerge: fix rough-set semantics for NullEQ - add a dedicated `NullEqual` rough-set operator instead of lowering to `Equal` - teach minmax index to evaluate nullable `NullEQ` with null-safe semantics - keep the old minmax-index compatibility path conservative when it cannot distinguish pure-null from mixed packs - add regression coverage for nullable packs and old minmax-index compatibility - update the new NullEQ test files introduced in pingcap#10726 to use copyright year 2026
ti-chi-bot
Bot
added
the
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds TiDB NullEQ ( ChangesNullEQ Push-Down Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dbms/src/Storages/DeltaMerge/Index/tests/gtest_dm_minmax_index.cpp`:
- Around line 2448-2455: The DAGQueryInfo construction in
gtest_dm_minmax_index.cpp is using the wrong overload and is missing required
parameters. Update the std::make_unique<DAGQueryInfo> call in the minmax index
test to match the full constructor signature used by ParseIn, including the
missing fts_query_info and used_indexes arguments in the correct order. Use the
DAGQueryInfo constructor and the surrounding ParseIn-style argument sequence as
the reference for locating and fixing this call.
In `@dbms/src/Storages/DeltaMerge/tests/gtest_dm_filter_parser_nulleq.cpp`:
- Around line 95-103: The DAGQueryInfo construction in the null-eq parser test
is missing required arguments, so update the std::make_unique<DAGQueryInfo> call
to pass both fts_query_info and empty_used_indexes in the correct position. Use
the existing local values and match the constructor signature in DAGQueryInfo
exactly so the test builds against the current API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fcd47c8c-fa93-4f5a-8ca5-a5a90c7d977b
📒 Files selected for processing (13)
dbms/src/Flash/Coprocessor/DAGUtils.cppdbms/src/Flash/Coprocessor/tests/gtest_tidb_null_eq_func.cppdbms/src/Functions/FunctionsComparison.cppdbms/src/Functions/tests/gtest_tidb_null_eq.cppdbms/src/Storages/DeltaMerge/Filter/NullEqual.hdbms/src/Storages/DeltaMerge/Filter/RSOperator.cppdbms/src/Storages/DeltaMerge/Filter/RSOperator.hdbms/src/Storages/DeltaMerge/FilterParser/FilterParser.cppdbms/src/Storages/DeltaMerge/FilterParser/FilterParser.hdbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cppdbms/src/Storages/DeltaMerge/Index/MinMaxIndex.hdbms/src/Storages/DeltaMerge/Index/tests/gtest_dm_minmax_index.cppdbms/src/Storages/DeltaMerge/tests/gtest_dm_filter_parser_nulleq.cpp
|
@windtalker: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Hi @windtalker and maintainers, |
|
Hi @yy782 , thanks a lot for your interest in TiFlash and for working on this issue. Sorry that we did not notice earlier that this issue had been assigned to you, while another implementation was already ongoing on our side. This caused two overlapping PRs for the same issue, and I apologize for the confusion. After reviewing both PRs, I would prefer to use #10946 as the base implementation. The main reason is that #10946 introduces a dedicated That said, your PR #10954 has very valuable test coverage, especially the richer type-combination tests and the fullstack end-to-end test. I think those tests would be a good supplement to #10946. So my suggestion is to keep #10946 as the core implementation, and incorporate the additional test coverage from #10954. We will make sure your contribution is properly recognized when doing that. Does this sound reasonable to you? |
|
Thanks @JaySon-Huang! I'm happy to collaborate. Please let me know how you'd like me to help integrate the tests. |
|
Hi @yy782, Thanks for being willing to collaborate. I think a good next step would be to use #10946 as the base implementation. You can first run: gh pr checkout 10946This will check out
A few notes:
After that, you can either force-push the updated changes to your current |
| private: | ||
| TiDB::TiDBCollatorPtr collator = nullptr; | ||
| std::shared_ptr<FunctionEquals> equals_function = std::make_shared<FunctionEquals>(); | ||
| }; |
There was a problem hiding this comment.
Seems that we can remove the collator from FunctionTiDBNullEQ
| private: | |
| TiDB::TiDBCollatorPtr collator = nullptr; | |
| std::shared_ptr<FunctionEquals> equals_function = std::make_shared<FunctionEquals>(); | |
| }; | |
| private: | |
| std::shared_ptr<FunctionEquals> equals_function = std::make_shared<FunctionEquals>(); | |
| }; |
| void setCollator(const TiDB::TiDBCollatorPtr & collator_) override | ||
| { | ||
| collator = collator_; | ||
| equals_function->setCollator(collator_); | ||
| } |
There was a problem hiding this comment.
| void setCollator(const TiDB::TiDBCollatorPtr & collator_) override | |
| { | |
| collator = collator_; | |
| equals_function->setCollator(collator_); | |
| } | |
| void setCollator(const TiDB::TiDBCollatorPtr & collator_) override | |
| { | |
| equals_function->setCollator(collator_); | |
| } |
| extern const int LOGICAL_ERROR; | ||
| } // namespace ErrorCodes | ||
|
|
||
| class FunctionTiDBNullEQ : public IFunction |
There was a problem hiding this comment.
| class FunctionTiDBNullEQ : public IFunction | |
| /// TiDB null-safe equal (`<=>`, mapped as `tidbNullEQ`). | |
| /// | |
| /// Difference from `equals`: | |
| /// - `equals` only compares nested values and does not interpret NULL markers. For nullable | |
| /// inputs, rows with NULL may still get 0/1 from placeholder nested data instead of SQL `=` | |
| /// semantics (where `NULL = x` is unknown/NULL). | |
| /// - `tidbNullEQ` applies TiDB `<=>` rules: `NULL <=> NULL` => 1, `NULL <=> x` => 0, and defers | |
| /// to `equals` only when neither side is NULL. The result is always non-nullable UInt8. | |
| class FunctionTiDBNullEQ : public IFunction |
| template <typename T> | ||
| RSResults MinMaxIndex::checkNullableNullEqualImpl( |
There was a problem hiding this comment.
| template <typename T> | |
| RSResults MinMaxIndex::checkNullableNullEqualImpl( | |
| /// Rough-check `col <=> value` for a nullable column using per-pack min/max. | |
| /// `value` must be non-null; `col <=> NULL` is handled by `checkIsNull` before reaching here. | |
| /// | |
| /// Unlike `checkNullableCmp` (SQL `=`), null-safe equal treats `NULL <=> value` as definitely false | |
| /// rather than unknown, so we never tag results with `has_null` (no `*Null` RSResult variants). | |
| template <typename T> | |
| RSResults MinMaxIndex::checkNullableNullEqualImpl( |
| RSResults results(pack_count, RSResult::Some); | ||
| const auto & minmaxes_data = toColumnVectorData<T>(column_nullable.getNestedColumnPtr()); | ||
| for (size_t i = start_pack; i < start_pack + pack_count; ++i) | ||
| { | ||
| if (details::minIsNull(null_map, i)) | ||
| { | ||
| if (has_null_marks[i] && !has_value_marks[i]) | ||
| results[i - start_pack] = RSResult::None; | ||
| continue; | ||
| } | ||
|
|
||
| auto min = minmaxes_data[i * 2]; | ||
| auto max = minmaxes_data[i * 2 + 1]; | ||
| auto value_result = RoughCheck::CheckEqual::check<T>(value, type, min, max); | ||
| if (has_null_marks[i] && value_result == RSResult::All) | ||
| results[i - start_pack] = RSResult::Some; | ||
| else | ||
| results[i - start_pack] = value_result; | ||
| } | ||
| return results; |
There was a problem hiding this comment.
| RSResults results(pack_count, RSResult::Some); | |
| const auto & minmaxes_data = toColumnVectorData<T>(column_nullable.getNestedColumnPtr()); | |
| for (size_t i = start_pack; i < start_pack + pack_count; ++i) | |
| { | |
| if (details::minIsNull(null_map, i)) | |
| { | |
| if (has_null_marks[i] && !has_value_marks[i]) | |
| results[i - start_pack] = RSResult::None; | |
| continue; | |
| } | |
| auto min = minmaxes_data[i * 2]; | |
| auto max = minmaxes_data[i * 2 + 1]; | |
| auto value_result = RoughCheck::CheckEqual::check<T>(value, type, min, max); | |
| if (has_null_marks[i] && value_result == RSResult::All) | |
| results[i - start_pack] = RSResult::Some; | |
| else | |
| results[i - start_pack] = value_result; | |
| } | |
| return results; | |
| /// Default to Some: conservative when min/max cannot prove a tighter bound. | |
| RSResults results(pack_count, RSResult::Some); | |
| const auto & minmaxes_data = toColumnVectorData<T>(column_nullable.getNestedColumnPtr()); | |
| for (size_t i = start_pack; i < start_pack + pack_count; ++i) | |
| { | |
| /// Min slot is NULL on indexes built before v6.4.0. For a pure-NULL pack, | |
| /// `NULL <=> value` is false for every row, so the pack can be skipped. | |
| if (details::minIsNull(null_map, i)) | |
| { | |
| if (has_null_marks[i] && !has_value_marks[i]) | |
| results[i - start_pack] = RSResult::None; | |
| continue; | |
| } | |
| auto min = minmaxes_data[i * 2]; | |
| auto max = minmaxes_data[i * 2 + 1]; | |
| auto value_result = RoughCheck::CheckEqual::check<T>(value, type, min, max); | |
| /// Non-null values may all equal `value`, but NULL rows still make `col <=> value` false. | |
| /// Downgrade All => Some so the pack is read and filtered row by row. | |
| if (has_null_marks[i] && value_result == RSResult::All) | |
| results[i - start_pack] = RSResult::Some; | |
| else | |
| results[i - start_pack] = value_result; | |
| } | |
| return results; |
3 participants
What problem does this PR solve?
Issue Number: close #5102
Problem Summary:
TiDB can push down NullEQ expressions, but TiFlash does not fully recognize and execute the pushed-down NullEQ semantics yet, including rough-set filtering in DeltaMerge.
What is changed and how it works?
This PR adds TiDB NullEQ function support in TiFlash, wires NullEQ pushdown handling, adds execution tests, and fixes DeltaMerge rough-set/min-max index semantics for NullEQ.
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
<=>) end-to-end:tidbNullEQfunction support and DeltaMerge filtering via thenull_equalrough filter.Bug Fixes
NULL-safe equality, including correct behavior for mixedNULL/non-NULLpacks.Tests
tidbNullEQ/null_equal, including negation and collator forwarding.